-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Set media viewers' settings menu dimensions with javascript #116
Conversation
src/lib/viewers/media/Settings.js
Outdated
this.settingsEl.style.width = `${menu.offsetWidth + 18}px`; | ||
// height = n * $item-height + 2 * $padding (see Settings.scss) | ||
// where n is the number of displayed items in the menu | ||
const sumHeight = [].reduce.call(menu.children, (sum, child) => { return sum + child.offsetHeight; }, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Jeremy mentioned in the other review - no need for { } or returns here.
const sumHeight = [].reduce.call(menu.children, (sum, child) => sum + child.offsetHeight, 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/lib/viewers/media/Settings.js
Outdated
* Set the menu dimensions depending on which menu is being shown | ||
* | ||
* @private | ||
* @param {Element} menu - The menu/submenu to use for sizing the container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{HTMLElement}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This fixes a bug for when the localized items in the menu would not fit in the menu's fixed width size set by CSS. This will also pave the way for adding more dynamic submenus, e.g. when there are a variable number of subtitles or audio tracks. We could have made the width/height dynamically adjust using width=auto or height=auto. But then CSS transitions don't work. So I resort to a small amount of javascript in this commit to adjust dimensions dynamically. Also, in order to have labels and values all be vertically aligned as a column, I use CSS to give a table layout (display: table). This brings an additional advantage for screen-readers - the label text (e.g. SPEED) is no longer seen as in-line with the value (e.g. Normal), so screen-readers will read them as separate words.
This fixes a bug for when the localized items in the menu would not fit
in the menu's fixed width size set by CSS. This will also pave the way
for adding more dynamic submenus, e.g. when there are a variable number
of subtitles or audio tracks.
We could have made the width/height dynamically adjust using width=auto
or height=auto. But then CSS transitions don't work. So I resort to
a small amount of javascript in this commit to adjust dimensions
dynamically. Also, in order to have labels and values all be vertically
aligned as a column, I use CSS to give a table layout (display: table).
This brings an additional advantage for screen-readers - the label text
(e.g. SPEED) is no longer seen as in-line with the value (e.g. Normal),
so screen-readers will read them as separate words.